Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make form events bubble again #19285

Closed
wants to merge 1 commit into from
Closed

Make form events bubble again #19285

wants to merge 1 commit into from

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Jul 8, 2020 •
edited
Loading

This PR changes form events, specifically submit and reset so that behave as native bubbling events. Today, with the modern event system, that means making them not use the capture phase.

There are a few issues though. We want to move towards removing the notion of have a special capture phase for certain events. One effort is to make them listen only to their target element, as we do in this PR: #19278. That works for all tests, apart from the recent regression test that @gaearon added, where we expect submit to be event replayed. We can't replay events that are only attached to their target, as that means we'll double-fire if we try to listen to them on the root too.

Moving submit and reset to bubble phase sound sensible then right? They natively bubble anyway according to DOM spec, and this includes all major browsers we support. The issue is that we have a Jest test and a reference PR that shows where this went wrong in the past:

#13462

Can we maybe go back and fix this problem internally? Then it would unblock this particular set of events and make them behave as they should do. It would also mean that we don't need to treat it as a target only event in #19278, and thus the test that @gaearon added should pass.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 8, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 8, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 04320fd:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Jul 8, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 04320fd

@sizebot
Copy link

sizebot commented Jul 8, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 04320fd

@trueadm trueadm mentioned this pull request Jul 8, 2020
Merged
@trueadm
Copy link
Contributor Author

trueadm commented Jul 8, 2020

Alternatively, we could change the Jest test for testing submit for event replaying so that it only tests this logic without event replaying. @sebmarkbage would this be a suitable compromise? I'd love your thoughts.

@bvaughn
Copy link
Contributor

bvaughn commented Jul 9, 2020

I don't feel like I have enough context to approve or reject this PR. Seems like the answer hinges on this question:

Can we maybe go back and fix this problem internally?

So then my question is: Who should make that decision? (That's the thing I really don't have context on- how much effort would it be to fix Workplace.)

@trueadm
Copy link
Contributor Author

trueadm commented Jul 9, 2020 •
edited
Loading

@bvaughn

So then my question is: Who should make that decision? (That's the thing I really don't have context on- how much effort would it be to fix Workplace.)

I'm looking into this now with @philipp-spiess. If we can't make this change though, the only other thing would be to not do this PR right now, and keep capture and reset events on the target. That would also mean removing them from event replaying, which @sebmarkbage would have to be happy with, until he can implement his ideas around changing replaying to the capture phase.

@trueadm trueadm mentioned this pull request Jul 9, 2020
Closed
@trueadm
Copy link
Contributor Author

trueadm commented Jul 9, 2020

I've put up an alternative approach: #19300

@gaearon
Copy link
Collaborator

gaearon commented Jul 9, 2020

which @sebmarkbage would have to be happy with

😄

@gaearon gaearon mentioned this pull request Jul 13, 2020
Merged
@trueadm trueadm closed this Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bvaughn bvaughn Awaiting requested review from bvaughn

@sebmarkbage sebmarkbage Awaiting requested review from sebmarkbage

@philipp-spiess philipp-spiess Awaiting requested review from philipp-spiess

@gaearon gaearon Awaiting requested review from gaearon

Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@trueadm @sizebot @bvaughn @gaearon @facebook-github-bot